Skip to content

Minor tooling and docs fixes#36

Merged
ttl-octave merged 4 commits into
gnu-octave:mainfrom
pganguli:misc-fixes
Jul 26, 2026
Merged

Minor tooling and docs fixes#36
ttl-octave merged 4 commits into
gnu-octave:mainfrom
pganguli:misc-fixes

Conversation

@pganguli

@pganguli pganguli commented Jul 24, 2026

Copy link
Copy Markdown
Contributor
  • compose.yaml: add the :z SELinux relabeling flag to the bind mount, so devel/compose.yaml works out of the box on SELinux-enforcing hosts (Fedora, RHEL, etc.) instead of silently hitting permission-denied errors.
  • check-local: run the container as the host user instead of root, so files created during make check-local aren't root-owned on the host afterward.
  • README.md: fix typos and trailing whitespace.

No functional changes to pkg-control itself.

@ttl-octave

Copy link
Copy Markdown
Member

Thanks for the contribution, @pganguli !

@MitchellThompkins, I would appreciate if you could have a brief look at the first two commits related to the test environment.

Comment thread devel/compose.yaml Outdated
@MitchellThompkins

Copy link
Copy Markdown
Contributor

Thanks for the contribution, @pganguli !

@MitchellThompkins, I would appreciate if you could have a brief look at the first two commits related to the test environment.

I left one comment but otherwise lgtm. I don't work on a SELinux machine so I didn't get bit by the :z thing but that looks like the canonical fix to me per stack overflow.

One thing though @ttl-octave, do you want to merge this into dev or straight into main? It's a minor fix to the local test environment which seems low risk but obviously up to you.

@ttl-octave

Copy link
Copy Markdown
Member

@MitchellThompkins, thanks for reviewing! I totally agree to directly merge this into main.

@pganguli

Copy link
Copy Markdown
Contributor Author

I don't work on a SELinux machine so I didn't get bit by the :z thing but that looks like the canonical fix to me per stack overflow.

Hi, yeah, I work on Fedora, so I had to fix that issue before I could even run anything using the docker image. I also tested to see if adding :z affects non-SELinux hosts, and it turns out to be a no-op.

@ttl-octave
ttl-octave changed the base branch from dev to main July 25, 2026 07:12
@ttl-octave
ttl-octave changed the base branch from main to dev July 25, 2026 07:35
@ttl-octave

Copy link
Copy Markdown
Member

I changed the base barnch to main but realized that this would add all the other commits that dev is ahead of main, so switched the base branch back to dev.
@pganguli, could you please rebase your branch misc-fixes to main and update the pull request?

pganguli added 4 commits July 25, 2026 15:56
* Without :z, Docker leaves the bind-mounted repo with its host
  SELinux label (e.g. user_home_t), which containers cannot access
  under Enforcing mode, causing check-local to fail with
  "Permission denied" on Makefile
* :z relabels the mount for shared access between host and container
* no-op on hosts without SELinux enabled
* gnuoctave/octave container runs as root by default, so files it
  creates in the bind-mounted repo (e.g. loose objects from
  check-ci's git stash create/git archive) end up owned by root on
  the host, blocking further git operations for the actual user
* pass HOST_UID/HOST_GID to compose.yaml's user: directive so the
  container writes files as the calling user instead
* Without a required check, running docker compose directly (bypassing
  make check-local) silently substitutes empty strings for HOST_UID/
  HOST_GID, producing a bogus "user: :" that fails confusingly
* :? makes docker compose fail fast with a clear error instead
@pganguli
pganguli changed the base branch from dev to main July 25, 2026 07:59
@pganguli

Copy link
Copy Markdown
Contributor Author

@pganguli, could you please rebase your branch misc-fixes to main and update the pull request?

Done!

@ttl-octave
ttl-octave merged commit 9a31f68 into gnu-octave:main Jul 26, 2026
@ttl-octave

Copy link
Copy Markdown
Member

If I run the docker via make check-local as my normal user, the installation of the control package fails because pkg tries to create the directory /.lcoal/share/octave... as normal user with missing permissions. If I run it as root (HOST_UID=0 and HOST_GID=0), everything works as expected. Should the docker run as root for the local check?

And another observation: I added 7.3.0 to the builds that are to be tested. This fails on the Github runner (also for 6.4.0) but passes when checking it locally.

@pganguli pganguli mentioned this pull request Jul 27, 2026
@pganguli

Copy link
Copy Markdown
Contributor Author

If I run the docker via make check-local as my normal user, the installation of the control package fails because pkg tries to create the directory /.lcoal/share/octave... as normal user with missing permissions. If I run it as root (HOST_UID=0 and HOST_GID=0), everything works as expected. Should the docker run as root for the local check?

Okay, it seems that if the user that you are running it under (say, docker) doesn't have an entry under /etc/passwd (thus getting an arbitrary UID), $HOME gets set to / and this problem occurs within the docker image. I'll fix this issue by explicitly setting $HOME to /workspace within the docker image.

And another observation: I added 7.3.0 to the builds that are to be tested. This fails on the Github runner (also for 6.4.0) but passes when checking it locally.

It seems that the relevant logs from the failing build are:

> Run make check-ci
Creating CI package dist directory target/control-4.2.2+-ci ...
  git archive ...
fatal: detected dubious ownership in repository at '/__w/pkg-control/pkg-control'
To add an exception for this directory, call:

	git config --global --add safe.directory /__w/pkg-control/pkg-control
fatal: detected dubious ownership in repository at '/__w/pkg-control/pkg-control'
To add an exception for this directory, call:

	git config --global --add safe.directory /__w/pkg-control/pkg-control
make: *** [Makefile:152: target/control-4.2.2+-ci] Error 128
Error: Process completed with exit code 2.

As the logs suggest, adding a safe.directory configure step fixes this.


I have opened #38 to fix both these issues. Sorry for the noise and thanks again for testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants